-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow skipping option and header resolution for api clients #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing, but I wonder whether the fix could be scoped down to a minimal amount of changes. The fact that this adds a new feature to work around the bug feels heavy-handed and potentially risky to me.
if tz != "" { | ||
headers[timeZone] = tz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alone looks like it's sufficient to solve the original issue (sending the blank Time-Zone header). How come additional changes to the functionality are needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that is not sufficient to solve the bug is because this line is explicitly telling go-gh
not to resolve the "Time-Zone" header and leave it set to an empty string. We could detect that in go-gh
and remove the "Time-Zone" header if it set to an empty string but that felt off since we have been following the rule that any user supplied values are not modified. Adding the SkipResolution
options feels like a cleaner solution and also addresses the confusion you correctly identified here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, but if there is going to be an additional config option, maybe it could be better documented? Right now I don't think it's clear what "resolution" is, since the user probably isn't familiar with the implementation of go-gh. Since the configuration option only affects whether default headers will be added, maybe the option could be called skipDefaultHeaders
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion, that is a much more descriptive name.
if tz != "" { | ||
headers[timeZone] = tz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, but if there is going to be an additional config option, maybe it could be better documented? Right now I don't think it's clear what "resolution" is, since the user probably isn't familiar with the implementation of go-gh. Since the configuration option only affects whether default headers will be added, maybe the option could be called skipDefaultHeaders
?
This PR adds the
SkipResolution
option toapi.ClientOptions
allowing the user to skip all automatic resolutions thatgo-gh
will do upon initializing an api client. This PR also changes the "Time-Zone" header to only be added if it was properly resolved, both changes are necessary for fixing 5978.Fixes cli/cli#5978
cc cli/cli#5935